Conversation
There was a problem hiding this comment.
Hit 'enter' accidentally mid-review, please ignore this until I have finished the review with a second submission! Sorry.
Overall looks decent! But I have some more considerable concerns (plus minor comments, see in-line):
-
There is the potential for module shadowing on
healpixhere - namely, because we have a module namedhealpix.py,import healpixcould pick up on that instead of the externalhealpixlibrary as intended (in certain cases, e.g. if whatever logic/application is running from thecfdirectory as CWD), which could lead to hard to diagnose issues somewhere down the line including unintended name clashes (probably mostly for us rather than the user but that's still bad).For example, when reviewing I was in the
cfdirectory to explore and would run into the following:Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/slb93/git-repos/cf-python/cf/field.py", line 5393, in healpix_increase_refinement_level or refinement_level > healpix_max_refinement_level() ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/slb93/git-repos/cf-python/cf/functions.py", line 3480, in healpix_max_refinement_level return healpix.nside2order(healpix._chp.NSIDE_MAX) ^^^^^^^^^^^^^^^^^^^ AttributeError: module 'healpix' has no attribute 'nside2order'
I suggest we rename the module to be safe. How about calling it
healpix_utils, e.g. to mirror the existingdask_utils? -
As indicated in a few illustrative in-line comments there, across the
cf/data/dask_utils_healpix.pymodule you use examples which specify the functions in question being called viacf.data.dask_utils.<function>, but none of those are exposed via that module andcf.data.dask_utils_healpixdoesn't get exposed either. I am wondering therefore if you meant to include the module or them individually in an appropriate__init.py__? (Or maybe you moved them out fromdask_utilsand forgot to update something there?)
| named `ESMF` with the old module name also being accepted for import, | ||
| version 8.7.0 or newer. This is easily installed via conda with | ||
| * `esmpy <https://earthsystemmodeling.org/esmpy/>`_, version 8.7.0 or | ||
| newer. This is easily installed via conda with |
There was a problem hiding this comment.
Not that this originates from this PR, but since we're touching it here: one should always avoid using terms like 'easy', 'simple' and similar because they are subjective and can put off users/readers if they don't find it easy as promised. I suggest just:
| newer. This is easily installed via conda with | |
| newer. This can be installed via conda with |
| * `healpix <https://pypi.org/project/healpix>`_, version 2025.1 or | ||
| newer. This package is not required to read and write HEALPix | ||
| datasets, but may be needed for particular manipulations with | ||
| HEALPix grids, such as creating latitude and longitude coordinates, | ||
| regridding, some changes to the refinement level, and some | ||
| collapses. |
There was a problem hiding this comment.
Hmmm... I am not 100% comfortable/happy with such a vague requisite. For one, it isn't then clear what tests would be expected to fail and therefore be skipped if healpix isn't available. Surely we can be more specific - or are there particular reasons we can't be I haven't picked up on?
| Define how to coarsening neighbourhood for each | ||
| axis. A dictionary key is an integer axis position, |
There was a problem hiding this comment.
There appears to be a missing word or (likely) set of words here, perhaps something like this was intended(?):
| Define how to coarsening neighbourhood for each | |
| axis. A dictionary key is an integer axis position, | |
| Define how large to set the coarsening neighbourhood for | |
| each axis. A dictionary key is an integer axis position, |
| ndim = self.ndim | ||
| for k in axes: | ||
| if k < -ndim or k > ndim: | ||
| raise ValueError("axis {k} is out of bounds for {ndim}-d data") |
There was a problem hiding this comment.
Missing the f-string prefix, plus a capital ideally for a bit more polish:
| raise ValueError("axis {k} is out of bounds for {ndim}-d data") | |
| raise ValueError(f"Axis {k} is out of bounds for {ndim}-d data") |
| # `None` then there are no vertical coordinates. | ||
| z_index: Any = None | ||
| # The original field/domain before any transformations are applied | ||
| # (such as creating lat/lon coordinates, or converting to from |
There was a problem hiding this comment.
| # (such as creating lat/lon coordinates, or converting to from | |
| # (such as creating lat/lon coordinates, or converting from |
(Or to/from?)
| f"Can't change HEALPix indexing scheme of {f!r}: " | ||
| "indexing_scheme in the healpix grid mapping coordinate " | ||
| "reference must be one of " | ||
| f"{healpix_indexing_schemes()!r}. Got {new_indexing_scheme!r}" |
There was a problem hiding this comment.
| f"{healpix_indexing_schemes()!r}. Got {new_indexing_scheme!r}" | |
| f"{healpix_indexing_schemes()!r}. Got {indexing_scheme!r}" |
|
|
||
| # Get the lat/lon coordinates | ||
| x_key, x = f.auxiliary_coordinate( | ||
| "Y", |
There was a problem hiding this comment.
| "Y", | |
| "X", |
| default=(None, None), | ||
| ) | ||
| y_key, y = f.auxiliary_coordinate( | ||
| "X", |
There was a problem hiding this comment.
| "X", | |
| "Y", |
| bounds that lie exactly on the north or south pole. If | ||
| `None` (the default) then the longitudes of such | ||
| points are determined by whichever algorithm was used | ||
| to create the coordinates, which will could result in |
There was a problem hiding this comment.
| to create the coordinates, which will could result in | |
| to create the coordinates, which could result in |
| including `numpy` and `Data` objects. The units of the | ||
| radius are assumed to be metres, unless specified by a | ||
| `Data` object. If the special value ``'earth'`` is | ||
| given then the default radius taken as 6371229 |
There was a problem hiding this comment.
| given then the default radius taken as 6371229 | |
| given then the default radius is taken as 6371229 |
Fixes #909
Requires NCAS-CMS/cfdm#371